-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[MLIR][Standalone] test Standalone against installed distribution #157944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
6cff170
to
6b77807
Compare
✅ With the latest revision this PR passed the Python code formatter. |
56936a0
to
efe7ace
Compare
mlir/test/Examples/standalone/test.install-distribution-dir.toy
Outdated
Show resolved
Hide resolved
92b9aa0
to
5fd5500
Compare
857cb13
to
dcc9b4a
Compare
78a1f90
to
d6011b6
Compare
-D CMAKE_EXE_LINKER_FLAGS="-no-pie" \ | ||
-D LLVM_ENABLE_WERROR=ON | ||
-D LLVM_ENABLE_WERROR=ON \ | ||
-DLLVM_INSTALL_UTILS=ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary with your new approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true it's not but it actually revealed (indirectly) a failure mode - because the monolithic
script is touched, all the projects are added, and the "layering issues" are revealed.
mlir/test/CMakeLists.txt
Outdated
get_property(MLIR_EXPORTS GLOBAL PROPERTY MLIR_EXPORTS) | ||
if(CLANG_ENABLE_CIR) | ||
list(APPEND MLIR_EXPORTS cir-opt cir-translate cir-lsp-server) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these? Having references to cir in MLIR looks like some layering issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a layering issue in the whole project (or possibly just CIR) then because this is the current error:
DLLVM_EXTERNAL_LIT="/home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/llvm-lit" -DMLIR_DIR="/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir" -DLLVM_DIR="/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/llvm" -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE="/usr/bin/python3" -DPython_EXECUTABLE="/usr/bin/python3" | tee -a /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/test/Examples/standalone/Output/test.install-dir.toy.tmp
2025-09-16T06:56:36.4951832Z # executed command: /usr/bin/cmake /home/gha/actions-runner/_work/llvm-project/llvm-project/mlir/examples/standalone -G Ninja -DCMAKE_CXX_COMPILER=/opt/llvm/bin/clang++ -DCMAKE_C_COMPILER=/opt/llvm/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DLLVM_EXTERNAL_LIT=/home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/llvm-lit -DMLIR_DIR=/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir -DLLVM_DIR=/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/llvm -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE=/usr/bin/python3 -DPython_EXECUTABLE=/usr/bin/python3
2025-09-16T06:56:36.4956077Z # .---command stderr------------
2025-09-16T06:56:36.4956844Z # | CMake Error at /usr/share/cmake-3.28/Modules/CMakeDetermineSystem.cmake:246 (configure_file):
2025-09-16T06:56:36.4957771Z # | No such file or directory
2025-09-16T06:56:36.4958268Z # | Call Stack (most recent call first):
2025-09-16T06:56:36.4958780Z # | CMakeLists.txt:2 (project)
2025-09-16T06:56:36.4959174Z # |
2025-09-16T06:56:36.4959483Z # |
2025-09-16T06:56:36.4960635Z # | CMake Error at /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir/MLIRTargets.cmake:4432 (message):
2025-09-16T06:56:36.4962030Z # | The imported target "cir-lsp-server" references the file
2025-09-16T06:56:36.4962659Z # |
2025-09-16T06:56:36.4963607Z # | "/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/bin/cir-lsp-server"
2025-09-16T06:56:36.4964887Z # |
2025-09-16T06:56:36.4965299Z # | but this file does not exist. Possible reasons include:
2025-09-16T06:56:36.4965753Z # |
2025-09-16T06:56:36.4966128Z # | * The file was deleted, renamed, or moved to another location.
2025-09-16T06:56:36.4966606Z # |
2025-09-16T06:56:36.4967001Z # | * An install or uninstall procedure did not complete successfully.
2025-09-16T06:56:36.4967499Z # |
2025-09-16T06:56:36.4967832Z # | * The installation package was faulty and contained
2025-09-16T06:56:36.4968249Z # |
2025-09-16T06:56:36.4969099Z # | "/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir/MLIRTargets.cmake"
2025-09-16T06:56:36.4970134Z # |
2025-09-16T06:56:36.4970407Z # | but not all the files it references.
2025-09-16T06:56:36.4971024Z # |
2025-09-16T06:56:36.4971292Z # | Call Stack (most recent call first):
2025-09-16T06:56:36.4972427Z # | /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir/MLIRConfig.cmake:37 (include)
2025-09-16T06:56:36.4973903Z # | CMakeLists.txt:9 (find_package)
2025-09-16T06:56:36.4974252Z # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.e., when LLVM_ENABLE_PROJECTS
is all projects, then MLIRTargets
includes CIR targets becaus CIR uses add_mlir_tool
:
add_mlir_tool(cir-lsp-server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the issue a mismatch between MLIRTargets and the global property MLIR_EXPORTS here? Should they just match? (and so the fix is rather in ensuring that they match rather than patching it back here)
Or maybe cir
should not use add_mlir_tool
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cir-lsp-server
is the only thing outside of mlir/
using add_mlir_tool
, cir-opt
is using add_clang_tool
, can we just update cir-lsp-server
?
(another reason for cir-opt / cir-translate to be in this list?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to just updating and removing CIR related code here.
) | ||
set_target_properties(check-mlir-build-only PROPERTIES FOLDER "MLIR/Tests") | ||
|
||
get_property(LLVM_EXPORTS GLOBAL PROPERTY LLVM_EXPORTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused?
if(CLANG_ENABLE_CIR) | ||
list(APPEND MLIR_EXPORTS cir-opt cir-translate cir-lsp-server) | ||
endif() | ||
add_custom_target( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be very clearly document here (the "why" and the "how")
COMMAND ${CMAKE_COMMAND} -E make_directory "${MLIR_STANDALONE_INSTALL_TEST_PREFIX}" | ||
COMMAND "${CMAKE_COMMAND}" | ||
-DCMAKE_INSTALL_PREFIX="${MLIR_STANDALONE_INSTALL_TEST_PREFIX}" | ||
-P "${MLIR_BINARY_DIR}/cmake_install.cmake" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think this command to be already pretty encompassing in itself, why are the other ones all needed below? (mlir-headers
for example?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you check the failing tests you will see the the union of COMMAND
s here is not sufficient. I am currently trying to figure out if this can actually work at all.
-DCMAKE_INSTALL_PREFIX="${MLIR_STANDALONE_INSTALL_TEST_PREFIX}" | ||
-P "${CMAKE_BINARY_DIR}/cmake_install.cmake" | ||
) | ||
list(APPEND MLIR_TEST_DEPENDS install-mlir-standalone-test-prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we only need this if testing the examples is enabled (that is LLVM_BUILD_EXAMPLES
is the guard I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable, but I'd echo I'm not opposed to just having this be on a buildbot (e.g., little bot that just does an install and runs a test using a shell script, doesn't need FileCheck or anything really) as I don't think the rollback was terribly bad (and rollbacks happen), and this could easily have been addressed earlier with a buildbot. E.g., I think the thing that resulted in rollback being slow was folks thinking "well is this due to my setup being wonky, what are the expectations here". While if we had a buildbot that failed, it would have been obvious, and we'd have rolled back faster. This being fast now is good and makes it viable for presubmit.
mlir/test/CMakeLists.txt
Outdated
get_property(MLIR_EXPORTS GLOBAL PROPERTY MLIR_EXPORTS) | ||
if(CLANG_ENABLE_CIR) | ||
list(APPEND MLIR_EXPORTS cir-opt cir-translate cir-lsp-server) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to just updating and removing CIR related code here.
d6011b6
to
bd9aabf
Compare
bd9aabf
to
8f0f30d
Compare
This PR adds a test that exercises standalone build;
test install
, which tests building the standalone example using an installed distribution.